Creating new API version for ADF#3270
Creating new API version for ADF#3270anuchandy merged 17 commits intoAzure:masterfrom hvermis:master
Conversation
2. Remove IR sharing related properties (this feature will not go GA for now) 3. Remove IR API 'removeNode', because IntegrationRuntimeNodes_Delete replaced it.
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
Automation for azure-sdk-for-pythonNothing to generate for azure-sdk-for-python |
Automation for azure-sdk-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeNothing to generate for azure-sdk-for-node |
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/queryPipelineRuns": { | ||
| "post": { | ||
| "operationId": "PipelineRuns_QueryByFactory", | ||
| "x-ms-examples": { |
There was a problem hiding this comment.
Should we name this operationId as PipelineRuns_ListByFactory given this operation is for listing pipe-line -runs List<PipelineRun> under a factory ? trying to be consistent with general patterns. We even have a linter rule for this.
#WontFix
There was a problem hiding this comment.
After discussing with my team leaving as Query #Resolved
| }, | ||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/pipelineruns/{runId}/queryActivityruns": { | ||
| "post": { | ||
| "operationId": "ActivityRuns_QueryByPipelineRun", |
There was a problem hiding this comment.
Should we name this operationId as ActivityRuns_ListByPipelineRun given this operation is for listing activity-runs List<ActivityRun> under a pieline run? #WontFix
There was a problem hiding this comment.
After discussing with my team leaving as Query #WontFix
| { | ||
| "$ref": "#/parameters/runId" | ||
| }, | ||
|
|
There was a problem hiding this comment.
Same as previous comment, consider using the operation id TriggerRuns_ListByFactory to be consistent with the pattern we established in SDKs #WontFix
There was a problem hiding this comment.
After discussing with my team leaving as Query #Resolved
| { | ||
| "$ref": "#/parameters/runId" | ||
| }, | ||
|
|
There was a problem hiding this comment.
using uuid (guid) is considered as ARM violation - refer this. I've seen other services with identity support uses string instead of uuid. Here is an example. But before making that change, let's see what ARM is recommending here. Since this is a new api-version Gaurav from ARM will be taking a look.
#WontFix
There was a problem hiding this comment.
I will change it to string. Just for information, why are GUIDs not recommended? #WontFix
There was a problem hiding this comment.
I just posted about this on slack channel for swagger - since these are for tenantId and principalId properties and not resource names, the format here is fine. In fact it gives an additional type validation on client side, so I will leave these two as GUIDs and add a linter exception. #Resolved
| { | ||
| "$ref": "#/parameters/runId" | ||
| }, | ||
|
|
There was a problem hiding this comment.
Annotate this with x-ms-enum like:
"x-ms-enum": {
"name": "<name-for-enum-type>",
"modelAsString": false | true
}
Do this for other enums as well. #Resolved
There was a problem hiding this comment.
Which one is this comment referring to? Hard to see. #Resolved
There was a problem hiding this comment.
I've searched through the whole file - the only ones that don't have x-ms-enum are the ones that contain the type name. These translate into a const string which should be intentional.
All others are modeled as enums. #Resolved
anuchandy
added
the
WaitForARMFeedback
anuchandy
removed
the
WaitForARMFeedback
|
Sync-ed with Hermine. There are some changes since last arm review (mainly query part was reviewed last time), hence requesting arm review. #Resolved |
anuchandy
added
the
WaitForARMFeedback
ravbhatnagar
left a comment
ravbhatnagar
left a comment
There was a problem hiding this comment.
Just one note about upgrade API. Others we had already reviewed and discussed.
| } | ||
| } | ||
| }, | ||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/upgrade": { |
There was a problem hiding this comment.
Upgrade should be modeled as a PUT. @hvermis - I dont recall we discussing this in our conversation. #Resolved
There was a problem hiding this comment.
Removed from swagger for now as per offline discussion. #Resolved
|
Signing off! |
ravbhatnagar
added
ARMSignedOff
5 participants
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-versionin the path should match theapi-versionin the spec).Quality of Swagger